Remove -alpha artifacts from runtime classpath of stable components #6944
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6944 +/- ##
============================================
- Coverage 90.10% 89.94% -0.17%
- Complexity 6602 6639 +37
============================================
Files 730 746 +16
Lines 19843 20016 +173
Branches 1955 1963 +8
============================================
+ Hits 17880 18003 +123
- Misses 1369 1419 +50
Partials 594 594 ☔ View full report in Codecov by Sentry. |
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeter.java
Outdated
Show resolved
Hide resolved
| try { | ||
| Class.forName("io.opentelemetry.api.incubator.trace.ExtendedTracer"); | ||
| return IncubatingUtil.createIncubatingTracer( | ||
| sharedState, instrumentationScopeInfo, tracerConfig); | ||
| } catch (Exception e) { | ||
| return new SdkTracer(sharedState, instrumentationScopeInfo, tracerConfig); | ||
| } |
There was a problem hiding this comment.
maybe?
| try { | |
| Class.forName("io.opentelemetry.api.incubator.trace.ExtendedTracer"); | |
| return IncubatingUtil.createIncubatingTracer( | |
| sharedState, instrumentationScopeInfo, tracerConfig); | |
| } catch (Exception e) { | |
| return new SdkTracer(sharedState, instrumentationScopeInfo, tracerConfig); | |
| } | |
| if (IncubatingUtil.isPresent()) { | |
| return IncubatingUtil.createIncubatingTracer( | |
| sharedState, instrumentationScopeInfo, tracerConfig); | |
| } else { | |
| return new SdkTracer(sharedState, instrumentationScopeInfo, tracerConfig); | |
| } |
There was a problem hiding this comment.
Yup yup that's my plan
There was a problem hiding this comment.
Tried this and a bunch of different variations of it.
The organization of methods is pretty finicky to get this to work, and it appears that by placing the isPresent() check in the same class file as the createIncubatingTracer method, the JVM tries to load the incubating classes required by createIncubatingTracer even if isPresent() is false.
Based on the stack trace, I think what is happening is the JVM reaches isPresent(), and tries to load all classes required by all methods in IncubatingUtil.
This problem occurs even if I use fully qualified class named in createINcubatingTracer, rather than imports.
Another thing to think about is how graal interacts with this increased use of reflection. I believe we'll have to add new reflect-config.json files to inform the graal runtime.
| @@ -23,4 +42,63 @@ void incubatingApiIsLoadedViaReflection() { | |||
| assertThat(MeterProvider.noop().get("test").counterBuilder("test")) | |||
| .isInstanceOf(ExtendedLongCounterBuilder.class); | |||
| } | |||
|
|
|||
| @Test | |||
| void incubatingLogSdk() { | |||
There was a problem hiding this comment.
Not sure why, but these new graal nativeTest woked without any additional reflect-config.json entries, despite new reflective access in the SDK via Class.forName. 🤔
Related to #6970.
Remove
-alphaartifacts (especiallyopentelemetry-api-incubator) from the runtime classpath of stable components.Approach is:
implementationtocompileOnlydependencies-alphaartifact is present on classpath (i.e. provided by the user) and toggle behavior based on whether it is or isnt-alphaartifacts are not provided by user, make sure main test suite runs without-alphaartifacts.-alphaartifacts are present, introduce new test suite including the-alphaartifacts, and test the impacted behaviorThis is related to a possible adjustment of our versioning guidelines. If we went in this direction, a user adding only stable dependencies could always run
./gradlew dependenciesand will not see any-alphaartifacts on the runtime classpath.